Updates to task / task list design - #553 #554 #555 #557

green-arrow · 2016-12-09T22:53:03Z

What's in this PR?

Redesigned API to account for the newly updated task list / task front end.

References

Total progress

Create DB changes to facilitate new design (Create schema for project task lists #554)
Automatically create task lists on project creation (Automatically create task lists when a new project is created #553)
Create APIs (Create APIs for task list #555)
Testing

Also added: * task list filtering for tasks API * automatically create task lists on project creation - #553 Still remaining: * Implementing APIs and views * Implementing tests

Modified project API / view to return task lists Modified tasks API to order by rank instead of inserted date Modified tasks API / view to return task list Added task list API / view to get all / single task lists

green-arrow · 2016-12-13T00:53:41Z

@joshsmith - this should be ready to review now. Feel free to pull in whoever else you think needs to review this.

joshsmith · 2016-12-13T07:33:32Z

🎉 thanks @green-arrow. Now that I've spent the past 8 hours or so exploring this problem top-to-bottom, thinking this looks pretty good, but now need to review more thoroughly.

I'm going to be playing around with a quick-and-dirty client implementation and then test out how well these will play together.

Stay tuned for a more thorough review.

If you wanted to move on to something else, please feel free.

joshsmith

Okay, have some thoughts we can discuss:

joshsmith · 2016-12-13T21:52:47Z

lib/code_corps/helpers/query.ex

  def project_id_with_number_filter(query, _), do: query

+  def task_list_id_with_number_filter(query, %{"id" => number, "task_list_id" => task_list_id}) do
+    IO.puts "YAY"


Needs removed.

joshsmith · 2016-12-13T21:54:11Z

lib/code_corps/helpers/query.ex

+    task_list_ids = task_list_ids |> coalesce_id_string
+    query |> where([object], object.task_list_id in ^task_list_ids)
+  end
+  def task_list_filter(query, %{"task_list_id" => task_list_id}) do


I don't think this one is necessary. See the id_filter for what I mean.

joshsmith · 2016-12-13T21:54:41Z

lib/code_corps/helpers/query.ex


  def sort_by_newest_first(query), do: query |> order_by([desc: :inserted_at])

+  def sort_by_rank(query), do: query |> order_by([asc: :rank])


Isn't this desc?

No, we want rank to be ascending (lower numbers are at the top of the list).

Ah, yes, you're right. sorry.

joshsmith · 2016-12-13T21:56:23Z

priv/repo/migrations/20161209192504_create_task_list.exs

+        |> put_assoc(:task_lists, TaskList.default_task_lists())
+        |> Repo.update
+
+        add_existing_tasks_to_inbox(project, hd(project.task_lists))


What is hd()?

hd/1 refers to the first item in the list, the head. (Also, tl/1 refers to the last item, the tail)

joshsmith · 2016-12-13T22:03:45Z

test/controllers/task_list_controller_test.exs

+        |> Enum.map(&Integer.parse/1)
+        |> Enum.map(fn({id, _rem}) -> id end)
+
+      assert ids == [task_list_2.id, task_list_1.id, task_list_3.id]


I thought we had some id assertion helpers in other controller tests.

joshsmith · 2016-12-13T22:05:10Z

test/views/task_list_view_test.exs

+    task_list_b = insert(:task_list, rank: 500, project: project)
+    task = insert(:task, rank: 1000, task_list: task_list_a)
+
+    rendered_json =  render(CodeCorps.TaskListView, "show.json-api", data: [task_list_a, task_list_b])


Why not just render the show here? Seems simpler.

I.e. just one task list, not two.

Good catch. I was working on seeing if I could work with position in the return and needed multiple items to verify. Since that's not how this works (rank is returned) we can go back to just one.

joshsmith · 2016-12-13T22:05:55Z

test/views/project_view_test.exs

+            "data" => [%{
+              "id" => task_list.id |> Integer.to_string,
+              "type" => "task-list"
+            }]


Prefer formatting like below with tasks.

joshsmith · 2016-12-13T22:06:15Z

test/views/task_view_test.exs

          "task-type" => task.task_type,
          "title" => task.title,
          "updated-at" => task.updated_at,
+          "rank" => task.rank


Alpha-order here.

joshsmith · 2016-12-13T23:01:43Z

web/controllers/task_controller.ex

  end
+
+  def record(%Plug.Conn{params: %{"task_list_id" => _task_list_id} = params}, _number_as_id) do
+    IO.inspect params


joshsmith · 2016-12-13T23:05:33Z

web/models/task.ex

    |> validate_inclusion(:task_type, task_types)
+    |> assoc_constraint(:task_list)
+    |> apply_position()
+    |> set_order(:position, :rank, :task_list_id)


This further makes me think order is the right name here.

green-arrow · 2016-12-14T00:55:00Z

@joshsmith - I gave a 👍 to all the things I fixed and left a couple comments in other places.

joshsmith · 2016-12-14T01:11:54Z

Looks great! Going to squash and merge if you're ready!

green-arrow · 2016-12-14T01:12:21Z

Have at it!

Added schema for tasks lists - #554

783df8b

Also added: * task list filtering for tasks API * automatically create task lists on project creation - #553 Still remaining: * Implementing APIs and views * Implementing tests

green-arrow added the in progress label Dec 9, 2016

green-arrow changed the title ~~Added schema for tasks lists - #554~~ Updates to task / task list design - #553 #554 #555 Dec 9, 2016

green-arrow added 4 commits December 12, 2016 09:34

Merge branch 'develop' into task-redesign

2bd2617

Fix seed data and task model tests.

77480ab

Added API and views for task lists

7d45f05

Modified project API / view to return task lists Modified tasks API to order by rank instead of inserted date Modified tasks API / view to return task list Added task list API / view to get all / single task lists

Merge branch 'develop' into task-redesign

741d0e4

green-arrow removed the in progress label Dec 13, 2016

green-arrow requested a review from joshsmith December 13, 2016 00:53

joshsmith added the needs code review label Dec 13, 2016

joshsmith modified the milestone: Redesign the tasks UI Dec 13, 2016

joshsmith reviewed Dec 13, 2016

View reviewed changes

joshsmith added needs submitter response and removed needs code review labels Dec 13, 2016

joshsmith mentioned this pull request Dec 13, 2016

Add task-list model code-corps/code-corps-ember#857

Merged

Address code review comments

106f78a

green-arrow added needs clarification and removed needs submitter response labels Dec 14, 2016

Change naming of 'rank' to 'order'

8ccbfaf

joshsmith removed the needs clarification label Dec 14, 2016

joshsmith approved these changes Dec 14, 2016

View reviewed changes

joshsmith merged commit f8a7f79 into develop Dec 14, 2016

joshsmith deleted the task-redesign branch December 14, 2016 01:17

joshsmith mentioned this pull request Dec 14, 2016

Create APIs for task list #555

Closed

3 tasks

This was referenced Dec 14, 2016

Automatically create task lists when a new project is created #553

Closed

Create schema for project task lists #554

Closed


		def sort_by_newest_first(query), do: query \|> order_by([desc: :inserted_at])

		def sort_by_rank(query), do: query \|> order_by([asc: :rank])

Updates to task / task list design - #553 #554 #555 #557

Updates to task / task list design - #553 #554 #555 #557

Uh oh!

Conversation

green-arrow commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

References

Uh oh!

green-arrow commented Dec 13, 2016

Uh oh!

joshsmith commented Dec 13, 2016

Uh oh!

joshsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

green-arrow commented Dec 14, 2016

Uh oh!

joshsmith commented Dec 14, 2016

Uh oh!

green-arrow commented Dec 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

green-arrow commented Dec 9, 2016 •

edited

Loading